Skip to content

fix(service): Propagate Sentry Hub correctly#368

Merged
lcian merged 8 commits intomainfrom
lcian/fix/bind-hub
Mar 12, 2026
Merged

fix(service): Propagate Sentry Hub correctly#368
lcian merged 8 commits intomainfrom
lcian/fix/bind-hub

Conversation

@lcian
Copy link
Member

@lcian lcian commented Mar 11, 2026

Currently, most of our Sentry traces and logs are mixed up, errors are not associated with the correct traces and logs, and errors/warnings logged inside the backends are missing the usecase and scopes tags.

This fixes that by creating a new hub, transaction and scope, and binding the new hub to the spawned tasks' future.
The new hub and transaction are necessary to handle cases where the execution of the task may outlive the web request.

@lcian lcian requested a review from a team as a code owner March 11, 2026 10:43
@lcian
Copy link
Member Author

lcian commented Mar 11, 2026

Maybe worth to add something about this to AGENTS.md or similar?
Although I'm not sure where it should be added exactly, feels a bit overkill to make it a top-level item in AGENTS.md as most of the time it won't be relevant.

let _ = tx.send(result);
drop(guard);
}
.bind_hub(Hub::current()),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see two potential problems with this:

  • Consider how this works for batch requests, which also call into spawn_metered. They would all share the same span stack - in this case I think it would be better to use Hub::new_from_top(Hub::current()) instead.
  • The task can outlive the web request, at which point the transaction is sent. We will be losing spans from the remaining operations that continue on the server.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Using new from top now
  2. Fixed this scenario as well. It's not beautiful but it works. I've tested it and this way you get the correct parent->child span relationship as well as the correct trace id association for errors and logs

@lcian lcian requested a review from jan-auer March 11, 2026 14:24
@lcian lcian changed the title fix(service): Bind Sentry Hub in concurrency::spawn_metered fix(service): Propagate Sentry Hub/scopes correctly Mar 11, 2026
@lcian lcian changed the title fix(service): Propagate Sentry Hub/scopes correctly fix(service): Propagate Sentry Hubcorrectly Mar 11, 2026
@lcian lcian changed the title fix(service): Propagate Sentry Hubcorrectly fix(service): Propagate Sentry Hub correctly Mar 11, 2026
Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

@lcian lcian marked this pull request as draft March 11, 2026 14:34
@lcian
Copy link
Member Author

lcian commented Mar 12, 2026

It works now, you can see the tests here https://sentry-sdks.sentry.io/issues/?project=4508694563782656&statsPeriod=1h
I added errors in 2 different places as well as a sleep inside the task (before the drop, after sending the response on the channel) to demonstrate it works in case 2 you pointed out here #368 (comment)

@lcian lcian marked this pull request as ready for review March 12, 2026 10:54
Copy link
Member

@jan-auer jan-auer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest to deduplicate the boilerplate and move it into spawn_metered.

@lcian lcian enabled auto-merge (squash) March 12, 2026 12:41
@lcian lcian merged commit 1831b0d into main Mar 12, 2026
21 checks passed
@lcian lcian deleted the lcian/fix/bind-hub branch March 12, 2026 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants